Conversation
ithinuel
force-pushed
the
async-pio
branch
2 times, most recently
from
62be79e to
c3f222c
Compare
| ($t:ident, $doc:expr, $offset:expr) => { | ||
| impl<S: ValidStateMachine, W: TransferSize> Wakeable for $t<S, W> { | ||
| fn waker() -> &'static IrqWaker { | ||
| static WAKER: IrqWaker = IrqWaker::new(); |
There was a problem hiding this comment.
First: I may be completely wrong, feel free to just tell me that I'm writing nonsense. I don't understand the internals of async rust well enough to confidently state that there is an issue with this code. I just don't understand how it is supposed to work.
If I understand this correctly, there is an issue with this static: It is shared with all instances of Rx (or Tx). There are no separate instances per monomorphization (https://doc.rust-lang.org/reference/items/static-items.html#r-items.static.generics). Therefore, if there are multiple state machines running in parallel, and one tries to await the Rx (or Tx) of more than one state machine at the same time, the second one to be polled will overwrite the waker of the first one.
Of course, this only matters if there actually are multiple wakers. With the trivial mock-executor used in the on-target-tests, where a single futures is called to completion from a single task, this wouldn't make a difference.
But if, for example, the two Rx are awaited from different CPU cores, this code may wake the wrong one, and the other one might stall.
There was a problem hiding this comment.
Well, you got it right >< I thought there would be 1 instance per monomorphisation. I'll fix this !!
There was a problem hiding this comment.
Sorry, I missed that you updated that code in the mean time.
The current version is better (no more sharing with all Tx/Rx instances).
But there's still only one instance for both interrupts. (The sharing over TransferSize isn't an issue because there can only be one instance per Tx/Rx.)
However, I wonder if the current concept of IrqWakers is sufficient. Isn't it possible that multiple tasks are waiting for the same interrupt? In that case, the interrupt would need some collection of wakers instead of the single one.
Alternatively, registering a new waker if another one was already registered could just wake the replaced one, like
https://docs.embassy.dev/embassy-sync/git/default/waitqueue/struct.GenericAtomicWaker.html does.
There was a problem hiding this comment.
I'm sorry I'm not very active these days, I'll be coming back to rp-hal when life settles a bit.
From what I remember:
- the life cycle of the wakers is such that a given waker can only be awaited on by 1 task only (they all require an &mut of the associated type/"singleton").
- The IRQ itself is user controlled. So if an application expects to await on several source for the same IRQ, it can call
::on_interrupt()for all of them in the interrupt handler.
With the implementation in this PR it would cause some spurious (yet legal) wake of the task. It might be possible to conditionally wake (only if the relevant flag is set). I'm adding a note to my backlog to check this 👍
This change saves us from reading ic_raw_intr_stat twice in the same loop.
One case is on read operations for 10bit I2C. First a write is issued with the address, then a restart and a the msb of the address is resent with this time the read flag. Because there is no data phase, the peripheral entered the active state but never switched to either Read or Write.
ithinuel
force-pushed
the
async-pio
branch
2 times, most recently
from
4fff5fb to
8039ffb
Compare
3 participants
I didn’t get time to test this on rp235x yet. If anyone has the bandwidth please try it and let me know <3
An example use case: